Skip to content

Conversation

CfirTsabari
Copy link
Contributor

@CfirTsabari CfirTsabari commented Mar 27, 2022

Rust std set_extension is either add an extension if none exists, or
edit the current one if such exists.

This caused a mishandling when user is using a name with a dot,
part of the name was treated as an extension, and be overwritten by
the different format extensions.

So manually adding a new dummy extension will cause the current code,
to behave as expected, since it will always overwrite the new dummy
extension and not part of the name.

@matthiasbeyer
Copy link
Member

matthiasbeyer commented Mar 29, 2022

Sorry but I do not understand why this change is necessary rather: How this change works. Could you explain a bit more (prefferably in the commit message)? And maybe provide tests for both bad cases and good cases? 🤔

Rust std `set_extension` is either add an extension if none exists, or
edit the current one if such exists.

This caused a mishandling when user is using a name with a dot,
part of the name was treated as an extension, and be overwritten by
the different format extensions.

So manually *adding* a new dummy extension will cause the current code,
to behave as expected, since it will always overwrite the new dummy
extension and not part of the name.
@CfirTsabari CfirTsabari force-pushed the cfirtsabari/doesn-t-work-with-dot-101 branch from 60c299d to 34ea07a Compare April 10, 2022 18:42
@CfirTsabari
Copy link
Contributor Author

CfirTsabari commented Apr 10, 2022

@matthiasbeyer hi,
changed the commit message.
regarding the tests I have added a case for the bad case but the good cases is all of the other existing tests.

@matthiasbeyer matthiasbeyer merged commit 44a1216 into rust-cli:master Apr 10, 2022
@matthiasbeyer
Copy link
Member

Thanks! I really like the commit message, thanks for improving it! ❤️ 🎉

epage added a commit that referenced this pull request Aug 20, 2025
The helper method is redundant (_introduced as a [bug fix in Mar
2022](#306), there is a much
simpler approach to append the extension.

-
[`as_mut_os_string()`](https://doc.rust-lang.org/std/path/struct.Path.html#method.as_mut_os_str)
has been available since Rust `1.70.0`, matching the MSRV of this crate
since the introduction of
[`OnceLock`](https://doc.rust-lang.org/std/sync/struct.OnceLock.html)
[in Oct 2024](#515) (_`0.14.1`
release_).
- Additionally revised the comment and included a conditional statement
to better communicate the intent for why this placeholder is necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants